On Fri, Jul 29, 2022 at 8:26 AM Peter Smith <smithpb2250@gmail.com> wrote: > > On Fri, Jul 29, 2022 at 11:55 AM houzj.fnst@fujitsu.com > <houzj.fnst@fujitsu.com> wrote: > > > > On Friday, July 29, 2022 7:17 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > During a recent review, I happened to notice that in the file > > > src/backend/catalog/pg_publication.c the two functions 'is_publishable_class' > > > and 'is_publishable_relation' used to be [1] adjacent in the source code. This is > > > also evident in 'is_publishable_relation' because the wording of the function > > > comment just refers to the prior function (e.g. "Another variant of this, taking a > > > Relation.") and also this just "wraps" the prior function. > > > > > > It seems that sometime last year another commit [2] inadvertently inserted > > > another function ('filter_partitions') between those aforementioned, and that > > > means the "Another variant of this" comment doesn't make much sense > > > anymore. > > > > Agreed. > > > > Personally, I think it would be better to modify the comments of > > is_publishable_relation and directly mention the function name it refers to > > which can prevent future code to break it again. > > I'd intended only to make the minimal changes necessary to set things > right again, but your way is better. > Yeah, Hou-San's suggestion sounds better to me as well. > > > > Besides, > > > > /* > > * Returns if relation represented by oid and Form_pg_class entry > > * is publishable. > > * > > * Does same checks as the above, > > > > This comment was also intended to refer to the function > > check_publication_add_relation(), but is invalid now because there is another > > function check_publication_add_schema() inserted between them. We'd better fix > > this as well. > +1. Here, I think it will be better to add the function name in the comments and keep the current order as it is. -- With Regards, Amit Kapila.